Skip to content

Conversation

@glyh
Copy link
Member

@glyh glyh commented Sep 16, 2025

This partially solves #17766

In short, the error reporting for zkApp command check short circuits. This PR make it so even if any error is reached, the logic will still go through all other checks to report all errors occurred in the input.

For the other part will open another PR

@glyh glyh requested a review from a team as a code owner September 16, 2025 12:56
@glyh glyh force-pushed the lyh/improve-account-update-error-report branch from 2651c27 to 607974e Compare September 16, 2025 13:00
@glyh glyh force-pushed the lyh/improve-account-update-error-report branch from 2dadd7e to a8dc366 Compare September 16, 2025 13:22
@glyh glyh added the enhancement Not big enough to be a feature, but is a smaller improvement label Sep 17, 2025
@cjjdespres
Copy link
Member

I'm not too sure about avoiding the short-circuiting. Are there any expensive checks done in this code?

I'm wondering if it would be okay to still short circuit but keep around the index of the update that failed this checking. We'd still at least report the index along with the error.

@glyh
Copy link
Member Author

glyh commented Sep 18, 2025

It seems hashing and I/O are the heavy stuff here.

                        ok_if_vk_hash_expected ~got:vk ~expected:vk_hash

And the invocation to find_vk might be heavy. Here's an example call site in Transaction_pool:

Zkapp_command.Verifiable.create ~failed:false
           ~find_vk:
             (Zkapp_command.Verifiable.load_vk_from_ledger
                ~get:(Mina_ledger.Ledger.get ledger)
                ~location_of_account:
                  (Mina_ledger.Ledger.location_of_account ledger) )
           zkapp_command
                Account_id.Table.update tbl account_id
                  ~f:(const @@ With_hash.hash prioritized_vk) ;

@glyh
Copy link
Member Author

glyh commented Sep 18, 2025

I'll make a follow up commit by making this error check feature optional. So end-user can swap them on if preferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Not big enough to be a feature, but is a smaller improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants